Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 288 and 279 #289

Merged
merged 35 commits into from
Nov 10, 2023
Merged

Fix 288 and 279 #289

merged 35 commits into from
Nov 10, 2023

Conversation

damianooldoni
Copy link
Member

@damianooldoni damianooldoni commented Nov 8, 2023

This PR solves:

Also, some examples of get_record_table have been updated as the last version of mica datapackage we are using do not contain observations made in quick succession. Same holds true for the correspondent unit-tests.

@damianooldoni
Copy link
Member Author

@MartijnUH: could you please check the two functions in your use cases?
@PietrH: you can maybe start review or waiting until automatic checks are :green

Co-Authored-By: Peter Desmet <[email protected]>
@PietrH
Copy link
Member

PietrH commented Nov 10, 2023

test_that("output is a matrix", {
cam_op_matrix <- get_cam_op(mica)
expect_true(is.matrix(cam_op_matrix))
})

It's a real shame testthat has no real expectation for matrices, expect_type() can't handle it, and since it's not a S3 or S4 object, neither can expect_s3_class() or expect_s4_class()

Copy link
Member

@PietrH PietrH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some questions out of interest for @damianooldoni to have a look at when it suits him. Any changes I felt necessary I've made myself. I looked at the functionality as best as I could, but will depend on @MartijnUH to check if it suits the teams needs.

Great job Damiano!

R/get_record_table.R Outdated Show resolved Hide resolved
Comment on lines 61 to 75
record_table <- get_record_table(mica)
expected_colnames <- c("Station",
"Species",
"n",
"DateTimeOriginal",
"Date",
"Time",
"delta.time.secs",
"delta.time.mins",
"delta.time.hours",
"delta.time.days",
"Directory",
"FileName"
)
testthat::expect_identical(names(record_table), expected_colnames)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
record_table <- get_record_table(mica)
expected_colnames <- c("Station",
"Species",
"n",
"DateTimeOriginal",
"Date",
"Time",
"delta.time.secs",
"delta.time.mins",
"delta.time.hours",
"delta.time.days",
"Directory",
"FileName"
)
testthat::expect_identical(names(record_table), expected_colnames)
expect_named(
get_record_table(mica),
c(
"Station",
"Species",
"n",
"DateTimeOriginal",
"Date",
"Time",
"delta.time.secs",
"delta.time.mins",
"delta.time.hours",
"delta.time.days",
"Directory",
"FileName"
)
)

deltaTimeComparedTo = "lastRecord"
)) %>%
nrow()
testthat::expect_true(nrow_delta_10000 < nrow_delta_0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you know about:

testthat::expect_lt()

These comparison expectations have slightly nicer failure messages

https://testthat.r-lib.org/reference/comparison-expectations.html

mica_dup <- mica
# create duplicates at 2020-07-29 05:46:48, location: B_DL_val 5_beek kleine vijver
# use 3rd observation as the first two are unknown or blank (= no animal)
mica_dup$data$observations[,"sequenceID"] <- mica_dup$data$observations$sequenceID[3]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use purrr::chuck for this, because I find it more readable. But it's a matter of preference. Good job documenting by the way!

purrr::pluck is careful, and will fail silently, purrr::chuck will trow (chuck) an error when the index doesn't exist, another advantage.

get_record_table(mica,
minDeltaTime = 60,
mica_dependent <- mica
mica_dependent$data$observations[4,"timestamp"] <- lubridate::as_datetime("2020-07-29 05:55:00")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to use lubridate::dmy_hms() for character vectors, is there an advantage to lubridate::as_datetime()?


### Session and camera IDs

You can specify the column containing the camera IDs to be added to the station names following the camtrapR's convention: `Station__CAM_CameraID`. Only the row names are shown:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why only show the row names?

"it must be one of the deployments column names."
)
)
camera_values <- package$data$deployments[[camera_col]]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally use purrr::chuck() for this so I throw an error when I try to access an index that doesn't exist. I'm not really sure what the base behaviour is in this case. Ok to leave like this.

output <- output %>%
dplyr::left_join(n_media,
by = "sequenceID"
)
testthat::expect_equal(output$len, output$n)
testthat::expect_equal(output$len, output$n_media)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

n_media is a integer vector, and len is a double vector. Is this intentional?

@PietrH PietrH merged commit cee8f65 into main Nov 10, 2023
8 checks passed
@PietrH PietrH deleted the fix-288-and-279 branch November 10, 2023 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants